Fixed Entra Integration and added Entra-protected MCP client and server sample projects #940
Conversation
- implemented OAuth token handling with `scope` support for Entra ID. - implemented On-Behalf-Of flow for secure downstream API calls (Microsoft Graph, and SharePoint). - Updated `README.md` for setup, usage, and troubleshooting.
| ["resource"] = resourceUri.ToString(), | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
@PederHP , the code below behaves as I intended, but it's not a good implementation. The issue here is that if I pass the resource to Entra as it's defined in the PRM (e.g.: http://localhost:7071/) it will never be validated by it and all token requests result in a BadRequest that in PS looks like this:
At the same time, if I update the configuration to the expected value (the server app registration client id), it's not validated by the provider:
I tried to find alternatives, but am also trying to keep changes to a minimum. Would appreciate if you could share your thoughts.
There was a problem hiding this comment.
We've had previous PR's (#617 (comment)) to add an Entra-based sample, but we haven't merged them, because Entra doesn't implement all the security features required by the MCP spec yet. Namely, Entra doesn't properly handle this "resource" parameter (RFC 8707) as required by the MCP spec.
MCP clients MUST include the resource parameter in authorization and token requests as specified in the Resource Parameter Implementation section
@localden explains why this requirement is important for security in https://den.dev/blog/mcp-authorization-resource/, so I don't think it's a good idea to violate the MCP spec here.
There was a problem hiding this comment.
@localden did request that we remove the MUST requirement for the client sending in modelcontextprotocol/modelcontextprotocol#1614, but I don't think we should merge this ahead of the actual protocol change assuming it happens.
And even if the resource parameter isn't required, I think we should at least try to send the resource parameter first, since it can certainly enhance security. And then after that we should fall back to using a scope as suggested in modelcontextprotocol/modelcontextprotocol#1614. If the AS cannot support either, the client should fail to authenticate as it does today.
There was a problem hiding this comment.
Was typing the below as I noticed your second comment above. Both you points make sense. Agree that this shouldn't be merged.
Do you need me to do anything here or just leave it? Also, any ETA on an updated package for the SDK? (there are some updates not included in the last release that it would be great to have).
My interpretation about the specifications:
So, RFC 8707 states that:
"In requests to the authorization server, a client MAY indicate the protected resource (a.k.a. resource server, application, API, etc.) to which it is requesting access by including the following parameter in the request."
and the MCP specification states that:
"MCP clients MUST implement Resource Indicators for OAuth 2.0 as defined in RFC 8707"
But in reality the interpretation is that the MCP specification says it has to be more restrictive than the RFC? I'm confused. Would that be NOT following the RFC.
PS: My personal preference would be to be more restrictive, I'm just skeptical of that becoming true for Entra any time soon (if at all). This would be a huge blocker for many, I believe.
RFC 8707 - Resource Parameter
MCP - Resource Parameter Implementation
2 participants
scopesupport for Entra ID.README.mdfor setup, usage, and troubleshooting.I updated the ClientOAuthProvider to be compatible with MS Entra and added sample projects for that scenario.
PS: This will fix issue 939.
Motivation and Context
It is very important for the SDK to work with MS Entra for authentication and authorization.
How Has This Been Tested?
I tested with my MS Entra tenant. The sample project works and is able to successfully authenticate, exchange the token, and call downstream apis (MS Graph and SharePoint) accordingly.
Breaking Changes
Don't think so.
Types of changes
Checklist
Additional context